Skip to content

fix!: Fix pagination support for IssuesService list methods #3984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
maishivamhoo123 wants to merge 8 commits into
base: master
Choose a base branch
from
Open

fix!: Fix pagination support for IssuesService list methods #3984

maishivamhoo123 wants to merge 8 commits into from

Conversation

@maishivamhoo123
Copy link
Contributor

@maishivamhoo123 maishivamhoo123 commented Feb 9, 2026 •
edited by gmlewis
Loading

BREAKING CHANGE: ListCursorOptions is removed from IssuesService.List and IssuesService.ListByRepo.

Updates: #3976.

Removed unsupported cursor pagination IssuesService.List and IssuesService.ListByRepo by deleting ListCursorOptions from issue option structs. Updated related unit tests accordingly.

@maishivamhoo123
Copy link
Contributor Author

maishivamhoo123 commented Feb 9, 2026

@gmlewis, @alexandear and @Not-Dhananjay-Mishra can you please review this PR?

@gmlewis gmlewis changed the title fix: add pagination support for IssuesService list methods fix!: Add pagination support for IssuesService list methods Feb 9, 2026
@gmlewis gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Feb 9, 2026
@gmlewis gmlewis changed the title fix!: Add pagination support for IssuesService list methods fix!: Fix pagination support for IssuesService list methods Feb 9, 2026
@codecov
Copy link

codecov bot commented Feb 9, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.52%. Comparing base (c5bf1bc) to head (cb15057).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3984      +/-   ##
==========================================
- Coverage   93.52%   93.52%   -0.01%     
==========================================
  Files         207      207              
  Lines       17593    17592       -1     
==========================================
- Hits        16454    16453       -1     
  Misses        938      938              
  Partials      201      201

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gmlewis
gmlewis approved these changes Feb 9, 2026
View reviewed changes
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @maishivamhoo123!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @zyfy29 - @Not-Dhananjay-Mishra

Not-Dhananjay-Mishra
Not-Dhananjay-Mishra reviewed Feb 9, 2026
View reviewed changes
Copy link
Contributor

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also SubIssueService.ListByIssue ( Point 3 in #3976 ) also accepts IssueListOptions as one of the parameters so we can also include that in BREAKING CHANGE description

alexandear
alexandear reviewed Feb 10, 2026
View reviewed changes
Not-Dhananjay-Mishra
Not-Dhananjay-Mishra reviewed Feb 10, 2026
View reviewed changes
github/issues.go
Comment on lines -249 to -252

// Add ListOptions so offset pagination with integer type "page" query parameter is accepted
// since ListCursorOptions accepts "page" as string only.
ListOptions
Copy link
Contributor

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra Feb 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't remove ListOptions. keep both (only for IssueListByRepoOptions)

Copy link
Collaborator

@gmlewis gmlewis Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Not-Dhananjay-Mishra - so are you saying that this PR should be completely closed since both iteration options are needed?

Or are you saying that this struct (IssueListOptions) needs to be split up for different methods?

Looking through this PR now, I'm seeing that this remove here of ListOptions is essentially the only breaking change, right?

How should we proceed with this one?

gmlewis
gmlewis requested changes Feb 10, 2026
View reviewed changes
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @maishivamhoo123!

gmlewis
gmlewis requested changes Feb 10, 2026
View reviewed changes
@Not-Dhananjay-Mishra Not-Dhananjay-Mishra mentioned this pull request Feb 10, 2026
Open
@maishivamhoo123
Copy link
Contributor Author

maishivamhoo123 commented Feb 10, 2026 •
edited
Loading

image when i am removing the pointer from

// type IssueListOptions struct { // Filter specifies which issues to list. Possible values are: assigned, // created, mentioned, subscribed, all. Default is "assigned". Filter string url:"filter,omitempty"`

// State filters issues based on their state. Possible values are: open,
// closed, all. Default is "open".
State string `url:"state,omitempty"`

// Labels filters issues based on their label.
Labels []string `url:"labels,comma,omitempty"`

// Sort specifies how to sort issues. Possible values are: created, updated,
// and comments. Default value is "created".
Sort string `url:"sort,omitempty"`

// Direction in which to sort issues. Possible values are: asc, desc.
// Default is "desc".
Direction string `url:"direction,omitempty"`

// Since filters issues by time.
Since time.Time `url:"since,omitempty"`

ListCursorOptions

// Add ListOptions so offset pagination with integer type "page" query parameter is accepted
// since ListCursorOptions accepts "page" as string only.
ListOptions

}`
Linting is failing. @gmlewis @Not-Dhananjay-Mishra

@Not-Dhananjay-Mishra
Copy link
Contributor

Not-Dhananjay-Mishra commented Feb 10, 2026 •
edited
Loading

@maishivamhoo123
Add these lines to .golangci.yml

go-github/.golangci.yml

Lines 290 to 294 in e5024aa

- IssueListOptions.Direction # TODO: Issues
- IssueListOptions.Filter # TODO: Issues
- IssueListOptions.Since # TODO: Issues
- IssueListOptions.Sort # TODO: Issues
- IssueListOptions.State # TODO: Issues

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 10, 2026

Linting is failing. @gmlewis @Not-Dhananjay-Mishra

OK, this is my fault for flip-flopping on my opinions about struct tags and pointers.
Let me see if I can address this in a separate PR.

@maishivamhoo123
Copy link
Contributor Author

maishivamhoo123 commented Feb 10, 2026

Linting is failing. @gmlewis @Not-Dhananjay-Mishra

OK, this is my fault for flip-flopping on my opinions about struct tags and pointers. Let me see if I can address this in a separate PR.

now everything is working fine Sir? can you please review the pr?

@gmlewis gmlewis mentioned this pull request Feb 10, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexandear alexandear Awaiting requested review from alexandear

@gmlewis gmlewis Awaiting requested review from gmlewis

1 more reviewer

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra Not-Dhananjay-Mishra left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). NeedsReview PR is awaiting a review before merging.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maishivamhoo123 @Not-Dhananjay-Mishra @gmlewis @alexandear